Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Speed up and clean up unit tests #310

Merged
merged 8 commits into from
Oct 12, 2023
Merged

Speed up and clean up unit tests #310

merged 8 commits into from
Oct 12, 2023

Conversation

Yurlungur
Copy link
Collaborator

@Yurlungur Yurlungur commented Oct 12, 2023

PR Summary

This is a long-overdue PR to improve both the performance and readability of our unit tests. I do the following:

  1. Split test_eos_unit into multiple smaller files. In particular, the tabulated EOSs are new separated into sesame and stellar collapse files. The lower-level tests for, e.g., root finding are in a separate file now, and the tests for vector EOS and EOS builder are in separate files. This dramatically improves readability, but it actually made compile times worse (for reasons will explain below).
  2. I split the eos_unit_tests executable into 3 executables, which are all registered with ctest. I try to organize around theme (such as analytic EOS, or infrastructure). This reduces link times, as we can link 3 different executables in parallel, all of which are smaller.
  3. I turn on the CATCH_CONFIG_FAST_COMPILE flag. I'm actually not sure what this does... remove some error handling, I think. It improves compile times by 20% or so.
  4. The reason compile times got slower when I split tests into more translation units is because each object was compiling the entire variant, even if the test was only testing, e.g., Gruneisen. As a final performance improvement, I selectively shrink the variant in each translation unit to contain only the models actually tested. In some cases, like testing modifiers or the EOS builder, this is still the whole variant, so the code path is exercised. But for single model tests, it's usually now that model + IdealGas.

With all these changes, I am able to shrink compile times (without closures, or fortran, or sesame) to 2m30s. This is a more than 2x improvement. I also think the code is substantially cleaner now.

There's one more improvement I'd like to make, probably in a separate PR, which I would like some input on. See issue #309 .

PR Checklist

  • Adds a test for any bugs fixed. Adds tests for new features.
  • Format your changes by using the make format command after configuring with cmake.
  • Document any new features, update documentation for changes made.
  • Make sure the copyright notice on any files you modified is up to date.
  • After creating a pull request, note it in the CHANGELOG.md file
  • If preparing for a new release, update the version in cmake.

@Yurlungur Yurlungur added Testing Additions/changes to the testing infrastruture build Something to do with the build system Performance Improving performance labels Oct 12, 2023
@Yurlungur Yurlungur self-assigned this Oct 12, 2023
Copy link
Collaborator

@jhp-lanl jhp-lanl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great change! Thank you for doing this!

doc/sphinx/src/contributing.rst Show resolved Hide resolved
test/CMakeLists.txt Show resolved Hide resolved
test/eos_unit_test_helpers.hpp Show resolved Hide resolved
@Yurlungur Yurlungur merged commit 70d370b into main Oct 12, 2023
4 checks passed
@Yurlungur Yurlungur deleted the jmm/fastertests branch October 12, 2023 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Something to do with the build system Performance Improving performance Testing Additions/changes to the testing infrastruture
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants